-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[WIP] Refactor _getcapture #6671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
it was intentionally done as own method, as too-fd capture is vastly more difficult than too-sys capture as the implementation will vastly differ and cant share logic, using a flag instead of a new implementation name seemed misleading |
@RonnyPfannschmidt |
I guess with regard to |
for the capture fixture, it seems like it would be helpful if it could be turned into one fixture that may be configured by a parameter/marker instead of having many mutually exclusive fixtures |
side-note, the more tricky implementation that's necessary for a tee-fd is potentially also a gateway to a pty/tee-pty capture backed |
Yes. |
That should be the long-term goal, given that there is the |
good point |
f0b83b2
to
c5ed3ab
Compare
Removing "[WIP]". |
c5ed3ab
to
2e29be9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -512,7 +532,11 @@ class NoCapture: | |||
__init__ = start = done = suspend = resume = lambda *args: None | |||
|
|||
|
|||
class FDCaptureBinary: | |||
class Capture: | |||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty class is here for type hinting only? Perhaps adding a quick one line docstring mentioning that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, or remove it.
This certainly needs another pass.
I've thought about pulling out the renaming/moving of the new classes only for now, given that combinations are not really needed currently anyway (also because of the "choices" validation).
It might make sense to pick that part into #6690, but there it appears to be unclear still how to do it (at least to me). Will try/look into that later, but might take some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks
@aklajnert it's not really clear to me how to proceed here (#6671 (comment)). |
@aklajnert I've pulled out #6765, without the generalisation of |
This refactors the Capture classes to be more flexible.
The new
--capture=tee-sys
(#6315)is rather a flag that could have been also done via
--capture-tee
, tolater also apply to
fd
.This was inspired by looking at it from the point of adding a "duplicate stderr
to stdout" mode (i.e. a single stream), which also is a new mode that can be
applied to both the fd and sys method.
Therefore this handles
--capture
now as a set of modes (split on dashes).This does not change any behavior for now, but can be refactored more, and
I might look into adding support for
tee-fd
then also.Ref: #6315 (comment)
Ref: #4597
Question: should there also be new fixtures for this, i.e.
capteesys
?That would result in 4 new fixtures then though:
"capteefd", "capteefdbinary", "capteesys", "capteesysbinary", and twice as much
again with "single" - i.e. it gets messy with
--fixtures
etc.So for this it might make sense to have a fixture that only works as a flag
then (
captee
), so that you would requestcapsys
andcaptee
./cc @csm10495